🦋 Changeset detected

Latest commit: c6b5d73

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ kdy1
❌ github-actions[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

🤖 This pull request has been linked to DevBird Task #1620

View the task details and manage the automated development workflow in DevBird.

Learn more about DevBird here or the announcement blog post here.

📋 DevBird Task Prompt

Fix #11255

This comment was automatically added by DevBird. You can disable this feature in DevBird Settings.

Claude finished @DeLino[bot]'s task —— View job

Code Review for PR #11256

Reviewing the fix for compress.comparisons side effects issue...

Todo List:

• Read and analyze the implementation changes
• Review test coverage and quality
• Analyze code quality and best practices
• Check for potential bugs and security issues
• Consider performance implications
• Address reviewer feedback about visitor pattern
• Provide comprehensive review feedback

Review Summary

This PR correctly addresses issue #11255 by preventing the compress.comparisons optimization from incorrectly transforming expressions with side effects. However, there are several implementation issues that need to be addressed.

Critical Issues

1. Visitor Pattern Concerns (Addresses kdy1's feedback)

The current implementation uses a recursive pattern matching approach rather than the visitor pattern, which is actually good practice for this use case. However, if a visitor approach were to be used, it would need several fixes:

• Missing noop_visit_type! macro: All visitor implementations in this codebase consistently use noop_visit_type!(fail); as the first line
• Scope boundary issues: The visitor would incorrectly flag side effects in nested functions/closures that don't affect the outer expression

2. Logic Bugs in Current Implementation

The recursive implementation has a critical flaw in scope handling:

Problem: crates/swc_ecma_minifier/src/compress/optimize/ops.rs:325-332

Expr::Call(CallExpr {
    callee: Callee::Expr(callee),
    args,
    ..
}) => {
    contains_update_or_assign(callee)
        || args.iter().any(|arg| contains_update_or_assign(&arg.expr))
}

This will incorrectly flag expressions like:

obj.method === obj.method

Where method is function() { ++i; }. The side effects inside the function don't affect the comparison's evaluation.

Recommended Fixes

Option 1: Fix Scope Boundaries (Preferred)

Add scope boundary checks to prevent traversing into:

• Function expressions (Expr::Fn)
• Arrow functions (Expr::Arrow)
• Class expressions (Expr::Class)
• Method definitions

Option 2: Use Visitor Pattern (If required by maintainer)

If visitor pattern is mandatory, implement it correctly with:

impl Visit for UpdateOrAssignChecker {
    noop_visit_type!(fail);
    
    fn visit_update_expr(&mut self, _: &UpdateExpr) {
        self.found = true;
    }
    
    fn visit_assign_expr(&mut self, _: &AssignExpr) {
        self.found = true;
    }
    
    // Stop at scope boundaries
    fn visit_function(&mut self, _: &Function) {}
    fn visit_arrow_expr(&mut self, _: &ArrowExpr) {}
    fn visit_class(&mut self, _: &Class) {}
}

Test Coverage Assessment

✅ Good Coverage

• Tests the exact issue case from Enabling compress.comparisons may lead to issues. #11255
• Verifies optimization is correctly skipped
• Tests both regular and mangle-only modes

❌ Missing Edge Cases

• No tests for scope boundaries (nested functions with side effects)
• No tests for other side effect patterns (assignments, compound assignments)
• No tests for complex nested expressions

Suggested additional test cases:

// Should NOT be optimized (current behavior)
console.log(arr[++i] === arr[++i]); 

// Should be optimized (currently might fail)
console.log(obj.method === obj.method); // where method = function() { ++x; }
console.log((() => ++x) === (() => ++x)); // arrow functions with side effects

Performance Considerations

✅ Efficient for Common Cases

• Early returns for simple expressions
• Minimal overhead for expressions without side effects

⚠️ Potential Issues

• Deep recursive calls could cause stack overflow on very nested expressions
• No memoization for repeated subexpressions

Security Assessment

✅ No security concerns identified - this is a correctness fix that prevents incorrect optimizations.

Code Quality

✅ Positives

• Clear function name and documentation
• Handles most expression types correctly
• Simple recursive approach is easy to understand

❌ Issues

• Missing scope boundary handling
• No tests for edge cases
• Function could be more robust

Final Recommendation

Request Changes - The core fix is sound, but the scope boundary issue needs to be resolved before merge. The current implementation may cause false positives that prevent valid optimizations.

Priority fixes:

1. Add scope boundary checks for functions, arrows, and classes
2. Add comprehensive tests for edge cases
3. Consider adding the noop_visit_type! macro if visitor pattern is adopted

Binary Sizes

Commit: 2571152

CodSpeed Performance Report

Merging #11256 will not alter performance

_{Comparing devbird/fix-compress-comparisons-side-effects (c6b5d73) with main (b56e008)}

Summary

✅ 138 untouched

Code Review for PR #11256

I've reviewed this pull request and here's my comprehensive feedback:

Overall Assessment

✅ Strong approval - This PR correctly fixes a critical bug where the minifier was transforming comparisons with side effects, altering program semantics.

Code Quality & Best Practices

Strengths:

• Clean implementation: The contains_update_or_assign() function uses recursive pattern matching following Rust and codebase conventions (ops.rs:297-361)
• Targeted fix: The check is correctly applied only when comparing identical expressions (x === x), not for all === to == conversions (ops.rs:47)
• Good code organization: Helper function is well-documented and properly scoped
• Follows CLAUDE.md: Code is written in English, avoids nightly-only features, and includes proper documentation

Suggestions:

1. Consider additional expression types: The function currently handles most cases, but might want to explicitly handle:

   • Expr::Yield - yield expressions can have side effects
   • Expr::Await - await expressions can have side effects
   • Expr::New - constructor calls can have side effects
   • Expr::TaggedTpl - tagged template literals can have side effects (function calls)

   These could potentially appear in computed property accesses like obj[await promise] or arr[new Counter()]

2. Function calls: Currently line 329-331 correctly checks for side effects in CallExpr, which is good since function calls can have side effects. However, this might be overly conservative - the optimization is only about detecting when the same expression is evaluated twice with different results. Function calls don't necessarily fall into this category unless they modify captured state. Consider if this is the intended behavior.

Correctness & Bug Prevention

Excellent fixes:

• ✅ Correctly identifies the root cause: duplicate evaluation of expressions with update operators
• ✅ The test case (issue_11255_side_effects) demonstrates the bug perfectly
• ✅ Properly handles nested expressions (binary, member, conditional, sequence, etc.)
• ✅ Handles both computed properties and optional chaining

Potential edge case:

• Array/Object literals with computed properties containing updates: [++i, ++i] - while this isn't a comparison, ensure this doesn't affect other optimizations

Performance Considerations

Analysis:

• ✅ The contains_update_or_assign() function is only called when e.left.eq_ignore_span(&e.right) is true, which should be rare
• ✅ Recursive approach is appropriate for AST traversal - matches codebase patterns
• ✅ Early returns on first match (using || short-circuiting) prevent unnecessary traversal
• ⚠️ Consideration: For deeply nested expressions, this could do significant work, but given it's only called on identical expressions, the performance impact should be minimal

Recommendation: Per CLAUDE.md #1 (prefer performance), this is acceptable since:

1. The check is only performed on a small subset of comparisons
2. The alternative (missing the optimization) would be worse than this conservative check
3. The function short-circuits on first finding

Security Concerns

✅ No security issues identified

• The fix prevents incorrect code transformations that could lead to logic bugs
• Actually improves security by preventing semantic changes
• No memory safety concerns
• No potential for injection or undefined behavior

Test Coverage

Strengths:

• ✅ Includes targeted test case for issue Enabling compress.comparisons may lead to issues. #11255
• ✅ Test correctly expects output to remain unchanged
• ✅ Includes both output.js and output.mangleOnly.js
• ✅ PR description indicates all existing comparing tests pass (12/12)

Suggestions for additional test cases:

1. Assignment operators:

   let x = 0;
   console.log((x = 1) === (x = 2)); // should not be optimized

2. Nested updates in different contexts:

   let i = 0;
   const check = (a, b) => a === b;
   check(arr[++i], arr[++i]); // should not be optimized

3. Update in optional chaining (already covered by the implementation):

   let i = 0;
   obj?.[++i] === obj?.[++i]; // should not be optimized

4. Verify safe cases still optimize:

   const x = 5;
   x === x; // should still optimize to x == x
   obj.prop === obj.prop; // without side effects, should still optimize

Changeset

✅ Changeset is properly formatted and includes both affected packages (swc_core and swc_ecma_minifier)

Minor Notes

1. Commit history: 8 commits for iterative fixes is fine for a bot-generated PR, but consider squashing if this were a human contributor
2. Documentation: The inline comment at ops.rs:295-296 is excellent

Final Recommendation

APPROVE with minor suggestions - This is a well-implemented fix for a real bug. The core logic is sound, test coverage demonstrates the fix works, and the implementation follows project conventions.

Action items (optional, can be addressed in follow-up):

• Consider adding test cases for assignment operators
• Evaluate if Yield, Await, New, and TaggedTpl should be included in the check
• Consider if function calls should really prevent this optimization (they don't have update semantics)

Great work on identifying and fixing this subtle minification bug! 🎉

PR Review: Fix compress.comparisons with side effects

Summary

This PR addresses a critical bug where the compress.comparisons optimization was incorrectly transforming comparisons containing expressions with side effects (like ++PC), changing program behavior. The fix correctly prevents optimization when expressions contain update or assignment operators.

✅ Strengths

1. Correct Problem Identification: The PR correctly identifies that the optimizer was treating Stack[Code[++PC]] === Stack[Code[++PC]] as identical operands when they're actually different due to side effects.

2. Well-Designed Helper Function: The contains_update_or_assign() function is comprehensive and handles many expression types:

   • Update expressions (++, --)
   • Assignment expressions
   • Binary expressions
   • Member expressions with computed properties
   • Call expressions
   • Conditional expressions
   • Optional chaining
   • Sequence expressions
   • Parenthesized expressions

3. Good Test Coverage: The PR includes a proper test case that validates the fix works correctly.

4. Clear Documentation: The changeset and PR description clearly explain the problem and solution.

🔍 Issues and Recommendations

1. Missing Check on Line 19 (High Priority)

Location: crates/swc_ecma_minifier/src/compress/optimize/ops.rs:19

There's another optimization at line 19 that also uses eq_ignore_span but doesn't check for side effects:

```rust
if e.left.is_ident() && e.left.eq_ignore_span(&e.right) {
let id: Ident = e.left.clone().ident().unwrap();
if let Some(t) = self.typeofs.get(&id.to_id()) {
match &**t {
"object" | "function" => {
// Replaces comparison with boolean
}
}
}
}
```

This optimization only checks is_ident(), so it shouldn't be affected by the member expression issue. However, for defensive programming, it might be worth adding a comment explaining why this case is safe (since identifiers alone can't have side effects).

2. Similar Issue in conditionals.rs (High Priority)

Location: crates/swc_ecma_minifier/src/compress/optimize/conditionals.rs:348

Found another optimization that may have the same issue:

```rust
// x ? x : y => x || y
if cond.test.is_ident() && cond.test.eq_ignore_span(&cond.cons) {
```

This only checks for identifiers, which is safe. But if there are other similar patterns elsewhere that check is_member() or other expressions with potential side effects, they would need the same fix.

Recommendation: Search for other uses of eq_ignore_span with .is_member() checks in the codebase to ensure all similar patterns are covered.

3. Missing Expression Types (Medium Priority)

The contains_update_or_assign() function doesn't check for:

• Expr::Yield: Yield expressions have side effects in generators
• Expr::Await: Await expressions can have side effects
• Expr::Call with all callee types: The current code only handles Callee::Expr, but not Callee::Super or Callee::Import
• Expr::New: Constructor calls can have side effects
• Expr::TaggedTpl: Tagged template literals execute functions and can have side effects

While these cases might be less common, they could still cause correctness issues. Consider adding:

```rust
Expr::Yield(..) | Expr::Await(..) | Expr::New(..) | Expr::TaggedTpl(..) => true,
```

4. Function Calls Not Covered (Medium-Low Priority)

Regular function calls (Expr::Call) are checked, which is good. However, the current implementation might be overly conservative - it prevents optimization even for pure function calls. This is probably the right trade-off for correctness, but it's worth documenting.

5. Performance Consideration (Low Priority)

The contains_update_or_assign() function is recursive and will traverse entire expression trees. For deeply nested expressions, this could add overhead. However, given that this is a minifier running at build time (not runtime), and the optimization it prevents would be incorrect, this is acceptable.

🧪 Testing

The test case is good but could be expanded:

• Test with -- operator
• Test with assignment operators (=, +=, etc.)
• Test with nested cases like `arr[foo()] === arr[foo()]` (function calls)
• Test that the first optimization site (line 19) still works correctly

📝 Style & Conventions

According to CLAUDE.md:

• ✅ Comments are in English
• ✅ No unstable features used
• ❓ Need to run `cargo fmt --all` (couldn't verify due to toolchain issue)
• ❓ Documentation is present (helper function is documented)
• ⚠️ Unit tests: The test is an integration test. Consider if a unit test for `contains_update_or_assign()` would be valuable

🎯 Recommendation

Approve with minor changes:

1. Add checks for Yield, Await, New, and TaggedTpl expressions
2. Handle all Callee variants in the Call case
3. Consider expanding test coverage
4. Run `cargo fmt --all` before merging
5. Verify no other `eq_ignore_span` patterns need similar fixes

The core fix is sound and addresses a real correctness issue. The suggested improvements would make it more robust, but the current implementation correctly fixes the reported bug.

Great work identifying and fixing this subtle optimization bug! 🎉